Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(trace): add more opcode handlings #128

Merged
merged 15 commits into from
Jul 24, 2022
Merged

fix(trace): add more opcode handlings #128

merged 15 commits into from
Jul 24, 2022

Conversation

0xmountaintop
Copy link

@0xmountaintop 0xmountaintop commented Jul 24, 2022

according to https://github.com/scroll-tech/zkevm-circuits/blob/scroll-dev-0714/bus-mapping/src/circuit_input_builder/access.rs#L114

related PR: scroll-tech/scroll-prover#29


question

In fact I don't know why I have traceCaller for CALL & CALLCODE, but looks like we should use traceContractAccount?

@0xmountaintop 0xmountaintop changed the title init fix(trace): add more opcode handlings Jul 24, 2022
@0xmountaintop 0xmountaintop marked this pull request as ready for review July 24, 2022 11:54
Copy link
Member

@noel2004 noel2004 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a little worry about the size of trace for contract code maybe large, and we may have recorded duplicated code in the block trace. Maybe it would be better to record all codes being accessed in "storageTrace" block or something similar? (It can be left as an optimization later)

@0xmountaintop
Copy link
Author

gonna merge first and run sushi-test.

will revert the changes for traceCaller vs traceContractAccount if failed

@scroll-dev scroll-dev merged commit eb11a84 into zkrollup Jul 24, 2022
@scroll-dev scroll-dev deleted the handle_op branch July 24, 2022 13:39
@0xmountaintop
Copy link
Author

0xmountaintop commented Jul 25, 2022

update:

The sushi test (which involves CALL) passes

@0xmountaintop 0xmountaintop restored the handle_op branch July 26, 2022 13:36
@0xmountaintop 0xmountaintop deleted the handle_op branch September 21, 2022 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants